feat: Regional Access Boundary Changes#891
feat: Regional Access Boundary Changes#891vverman wants to merge 12 commits intogoogleapis:trust_boundaryfrom
Conversation
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/baseexternalclient.ts
Outdated
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Show resolved
Hide resolved
packages/google-auth-library-nodejs/src/auth/externalAccountAuthorizedUserClient.ts
Outdated
Show resolved
Hide resolved
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
I made it so for ease of testing the getRegionalAccessBoundaryUrl logic. For ex: in the base external client we have scenarios where we want to test if the right url is returned or error is thrown in case of workload, workforce or a null audience.
There was a problem hiding this comment.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
There was a problem hiding this comment.
@vverman +1 to using @internal if you can verify that it's omitted from docs and transpiled classes.
packages/google-auth-library-nodejs/src/auth/regionalaccessboundary.ts
Outdated
Show resolved
Hide resolved
238aed8 to
f40a3e1
Compare
…e requestAsync level. RAB urls now have googleapis as universe domain.
e3e00da to
3427c0e
Compare
…been excluded and test suite updated.
…504 are the only retryable look up failures.
feywind
left a comment
There was a problem hiding this comment.
I don't see anything too weird in this, I guess let me know when you've got one that merges against main?
| if (res && res.status === 400) { | ||
| const data = res.data as {error?: {message?: string}; message?: string}; | ||
| const message = | ||
| data?.error?.message || data?.message || error.message || ''; |
There was a problem hiding this comment.
?? might be more idiomatic now, unless e.g. data?.message could be empty but not null/undefined.
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
I'd go ahead and put an @private on it, as that will cause it to be omitted from docs, but it will still show up in the transpiled class. Anyone digging in the source can see the @private to know they shouldn't use it.
Actually, it looks like @internal omits it from any exported .d.ts, so that may be enough. I'd double-check the output though, to see what it makes.
| } | ||
|
|
||
| protected async getTrustBoundaryUrl(): Promise<string> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string> { |
There was a problem hiding this comment.
Same comment re protected -> public.
| } | ||
|
|
||
| protected async getTrustBoundaryUrl(): Promise<string> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string> { |
| * Triggers an asynchronous regional access boundary refresh if needed. | ||
| * @param accessToken The access token to use for the lookup. | ||
| */ | ||
| private maybeTriggerRegionalAccessBoundaryRefresh(accessToken: string) { |
There was a problem hiding this comment.
Absolutely nitpicky, but I like to try to put an explicit return type on void methods just to make sure that you notice if something changes internal to it.
There was a problem hiding this comment.
Done, thanks for the suggestion!
packages/google-auth-library-nodejs/test/test.baseexternalclient.ts
Outdated
Show resolved
Hide resolved
| '{universe_domain}', | ||
| this.universeDomain, |
There was a problem hiding this comment.
SERVICE_ACCOUNT_LOOKUP_ENDPOINT no longer has a placeholder for universedomain.
The tests such as test/test.authclient.ts and test/test.compute.ts should be updated to remove the unnecessary replace too.
| * @param url The URL to check. | ||
| */ | ||
| private isGlobalEndpoint(url: string | URL): boolean { | ||
| const hostname = url instanceof URL ? url.hostname : new URL(url).hostname; |
There was a problem hiding this comment.
new URL(url) will throws a TypeError if the url string is a relative URL, or a malformed one.
We need to gracefully handle those cases by assuming they are global.
| /** | ||
| * Maximum cooldown period for RAB lookup failures. | ||
| */ | ||
| const RAB_MAX_COOLDOWN_MILLIS = 24 * 60 * 60 * 1000; |
| * returned response. | ||
| * @param opts The HTTP request options. | ||
| * @param reAuthRetried Whether the current attempt is a retry after a failed attempt due to an auth failure. | ||
| * @param retryWithoutRAB Whether the current attempt is a retry after a failed attempt due to a stale regional access boundary. |
There was a problem hiding this comment.
Since we no longer have the reactive refresh in this PR, this parameter should be removed too.
| * returned response. | ||
| * @param opts The HTTP request options. | ||
| * @param reAuthRetried Whether the current attempt is a retry after a failed attempt due to an auth failure. | ||
| * @param retryWithoutRAB Whether the current attempt is a retry after a failed attempt due to a stale regional access boundary. |
There was a problem hiding this comment.
Unused parameter: Since we no longer have the reactive refresh in this PR, this parameter should be removed too.
| * @throws {Error} If the URL cannot be constructed for a compatible client. | ||
| */ | ||
| protected async getTrustBoundaryUrl(): Promise<string | null> { | ||
| public async getRegionalAccessBoundaryUrl(): Promise<string | null> { |
There was a problem hiding this comment.
@vverman +1 to using @internal if you can verify that it's omitted from docs and transpiled classes.
| async getRequestHeaders(url?: string | URL): Promise<Headers> { | ||
| const headers = (await this.getRequestMetadataAsync(url)).headers; | ||
| const {headers, isIDToken} = await this.getRequestMetadataAsync(url); | ||
| if (!isIDToken) { |
There was a problem hiding this comment.
idtoken client overrides getRequestMetadataAsync is not returning isIDToken=true.
IIUC, IdTokenClient inherits the default getRegionalAccessBoundaryUrl (which returns null), so RAB lookup happen for idtoken and we won't attach the header anyway, but I think setting this flag explicitly ensures we skip the RAB logic entirely and avoids unnecessary checks and future-proofs against changes in the base classes.
Contains changes for the feature Regional Access Boundary (Previously Called Trust Boundaries).
The following are salient changes: